fix(sdk): make pending-approval consumption atomic - #1890
Conversation
|
Heads-up on the red |
ra-co88
left a comment
ra-co88
left a comment
There was a problem hiding this comment.
Verdict: approve — atomic approval consumption, correctly designed with honestly-documented residuals. (Comment review: GitHub blocks formal self-approval on your own PR.)
Supersession check (upstream/main @ 2dc399e): no consumePending/atomic-consumption path exists upstream — pending-approval.ts upstream still uses the read-then-delete window. Not superseded; the double-execution race is open upstream.
The mechanism is the right one:
compareAndDelete(namespace, key)with fail-closed contract on theBlobStoreinterface: true iff this caller's delete removed an existing record. Single winner by construction on every backend:- FumaDB: get+delete inside
fuma.transaction(real BEGIN/COMMIT serialization — the same seam the credential/policy writes use). - In-memory: one synchronous Map op (atomic in JS's model — the comment correctly notes no fiber can interleave).
- R2: no conditional delete exists, so it's isolate-scoped single-winner — claim-set gate closing the intra-isolate await window, plus head-then-delete. The residual (cross-isolate last-writer-wins) is documented in the PR body, not hidden: tolerated because the caller pattern is single-consume-per-approval with idempotent null-on-re-consume. This is the honest-residual discipline the framework requires — platform limits stated, not papered over.
- FumaDB: get+delete inside
- The consume path gates on the atomic result, so the loser sees absent-after-commit and does not execute.
Test discipline holds: the 128-line atomicity suite includes the discriminating negative controls (loser path, absent-after-commit, re-consume ⇒ null) rather than only the winner's happy path.
CI caveat, not blocking: the single failing shard (E2E cloud 13of16, run 2026-08-30) is the same cap-eviction openSession flake #1895 fixes; this branch predates it and touches no session code.
Good to merge.
2 participants
What
Pending-approval consumption is atomic: the blob that marks a pending approval is removed with a compare-and-delete whose single winner is the consumer.
BlobStoregainscompareAndDelete(namespace, key)— true iff this caller's delete removed an existing record.Why
Approvals are single-use decisions. A read-then-delete pair has a window where two concurrent requests both read the pending approval and both proceed — double execution on one human decision. The compare-and-delete contract guarantees exactly one concurrent caller observes true; the loser sees absent-after-commit.
What changed
BlobStore.compareAndDeletecontract (fail-closed semantics documented on the interface).fuma.transaction(real BEGIN/COMMIT on libSQL/Postgres — the serialization is the guarantee).Test plan
Atomicity suite: concurrent consumers — exactly one wins; loser observes absent; no double-execution path. 4 tests green against current main.